Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added --disable-init flag to fud xclbin stage #1085

Merged
merged 3 commits into from
Jul 7, 2022
Merged

Conversation

nathanielnrn
Copy link
Contributor

Fixes #1080, which is part of #1072

@rachitnigam
Copy link
Contributor

LGTM

@sampsyo
Copy link
Contributor

sampsyo commented Jul 7, 2022

Seems good if it works! Just thinking through the implications here for a sec… this pass disables all our "automatic initialization" stuff when generating Verilog but only for Xilinx execution. I hope that none of the code is relying on any kind of initialization… @andrewb1999's original advice in #1071 suggests that it was not necessary to get things working:

It's a bad idea to initialize logic (especially resets) that are used as wires rather than registers. I just commented out the entire initial block in the verilog which prevented issues with std_regs not being reset properly (currently none of the logic defined in the main module is being used as a register so this is fine to do).

But I wonder if this doesn't actually indicate a deeper problem. Namely, perhaps the initialization we're doing is wrong not just in a Xilinx context but for all Verilog generation—and we just need to back off on some of that altogether. It would take a little more digging into how we currently initialize things to know for sure, but perhaps there's a subtler change to be made here.

I would still recommend (1) forging ahead with this PR as-is, if it actually seems to improve things for Xilinx execution, and then (2) revisiting this broader "is our initialization stuff correct?" question later, with a goal toward removing the special --disable-init treatment in this stage.

@andrewb1999
Copy link
Collaborator

Yeah to be clear I think this is an issue with the verilog initialization in all cases, not just Xilinx. I think this is a case where different simulators/compilers will behave differently on something akin to undefined behavior in the systemverilog spec.

A good example of this behavior in the fsm_reset logic. It appears in three places in the dot-product.sv file, declaration, initialization to 0, and combinational assignment to the reset wire (assign fsm_reset = reset). Initialization only really makes sense in the context of registers (aka logic that is assigned to sequentially in an always block). In the Xilinx simulator, fsm_reset is always 0, the value it was initialized to, instead of getting set to 1 when reset is 1. I think this is some case of Xilinx getting confused because in one place it looks like we use fsm_reset as a register (initialization) and in another place we use it as a wire (combinational assignment with assign).

Other simulators may handle this situation differently, but it is certainly the case that if we remove the initialization of fsm_reset, all simulators will behave the same by making fsm_reset the same wire net as reset.

That being said, I agree with @sampsyo that this PR is a good starting place and we can come back to fixing the core issue with initialization later.

nathanielnrn and others added 2 commits July 7, 2022 14:51
This disables all automatic initialization when generating verilog, but
only for Xilinx execution. At some point thid may be revisited to a
broader "is initilization correct."

Previously, behavior was wonky because we were treating certain signals
as both registers (in initilization) and wires (later on).
* Changed axi manager port width to 512

* Added comments regarding kernel width

Co-authored-by: Nathaniel Navarro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove initialization of wires
4 participants